Can one of the admins verify this patch?

ok to test

I'm a little surprised that we left that pointer-dereference assignment in all those loops, but I guess it just got missed.

I double checked MPI-4.0 to make sure that the len argument isn't an INOUT, and it doesn't appear to be. MPI-4.0 p235:13-16 says:

The len argument ... is passed in as a reference for reasons of compatibility with Fortran.

@bosilca Can you verify?

@jsquyres the Git commit checker did not run (even if you issue the bot command`) because this is a new contributor (!), so I had to manually hit a 'Approve and Run' button (iirc) in the web interface.

@gkatev thanks for the patch!

Out of curiosity, did you make any performance measurements (with Open MPI compiled for production - e.g. -O2 or more aggressive)?
Though the patch looks correct at first glance, I would naively suspect the compiler optimize these pointer dereferences, ending up generating the same code with or without this patch.

This might be an aliasing issue: the compiler cannot assume that count isn't aliased by another pointer, unless it's marked as restrict. Hence the repeated load from memory.

@ggouaillardet I think I am not seeing a difference with O2/O3, but am I doing it correctly like this?
CFLAGS=-O2 make -C ompi
Or is there a config-time or otherwise way to tune the optimization level?

@gkatev I believe you'll need to pass in CFLAGS at configure time for it to have any effect.

you can check with make -V2 to see it got passed in.

FWIW in past projects I have seen these little optimizations making a difference.

Compilers should be smart enough, but can't always rely on that.

Check the assembly at godbolt for 4 different versions of the sum int32_t. Depending on the compiler, the generated code is slightly different. clang seems to assume the pointer to count is restrict and always generates the same optimized code, while gcc is taking a very cautious approach. In general the difference is reloading the *count from the cache instead of working on it from a register.

Conclusion, to remove the compiler from the equation and always be sure the code is as optimal as possible either we add restrict or we do the trick proposed here (temporary variable).

However, I think I would rather have the code with the following pattern:

void
ompi_op_base_2buff_sum_int32_t(const void *in, void *out, int *count)
  {
      int i;
      int32_t *a = (int32_t *) in;
      int32_t *b = (int32_t *) out;
      for (i = *count; i > 0; i--) {
          *(b++) += *(a++);
      }
  }

@bosilca Just curious: why keep it incrementing through a pointer dereference?

Not sure I understand the question. Are we talking about count or about the other pointers ?

@bosilca You're right... my question was non-sensical. Disregard!

I thought the point of @bosilca 's comment was to walk backwards (i--) instead of forward (i++), which may be more cache efficient (assuming that an application wrote the last bytes last and will access again from the first bytes)

Ahh never mind, the pointers are still walking forward 🤦

I created #9719 as an alternative to this. While digging in the code I also noticed we are not using the 3buff MPI_Op so I took the opportunity to remove all support.

FYI, for curiosity's sake I checked with make V=2, and it appears that all commands involving op_base_functions.c do contain -O3. (by default, without explicitely asking for it during configuration/compilation) -- and therefore all my tests here should have contained it.

@devreal you are absolutely right about pointer aliasing! and that makes the patch necessary to improve performance when the compiler chooses to generate correct code.

We merged #9719 and #9867 proposes removing the 3-buf variants. Should this PR now be closed?